How would you suggest handling this case in C?

We have a message m that equal to the empty bytestring? This is tested in the test vectors accompying this file here.

The python implmentation allows us to represent 2 different states that are (perhaps?) semantically mean the same thing is my understanding. Here are the 2 cases

1. m is not present (encoded as 00) with the prefix
2. m is present, but its the empty bytestring (encoded as 0100) with the prefix.

This can be represented in the type system of higher level languages like C++, Python, Rust, Scala etc.

From looking at the API on zkp, it seems like this wouldn't be possible to represent?

In C this must be modeled explicitly as an optional bytes type, otherwise we cannot distinguish
“m not present” (00) from “m present but empty” (0100).

I recommend representing this as { uint8_t *ptr; size_t len; bool is_present; } and encoding based on is_present.

Collapsing these cases would break the test vectors and change the hash domain.

#sc

We distinguish between a zeroed byte array and an empty byte string. Thus, m being absent (empty_bytestring) is different from m being equal to zero bytes (of any length), and we want to generate distinct nonces for these cases.

Yes, the current the zkp API doesn't add the message prefix correctly which needs to be fixed.

I agree with @vitrixLab, we can model this in C with an is_present variable, Musig2 does exactly this.

unsigned char msg_present;
msg_present = msg32 != NULL;
secp256k1_sha256_write(&sha, &msg_present, 1);
if (msg_present) {
    secp256k1_nonce_function_musig_helper(&sha, 8, msg32, 32);
}

The same function is also invoked in the DeriveInterpolatingValue() algorithm, which is called through the following chain:
ValidateSignersCtx()->DeriveThreshPubkey()->DeriveInterpolatingValue().
Is there a compelling reason to perform this check at the top level, or could the ID duplication check be retained solely at a lower level within DeriveInterpolatingValue()?

In addition, the ValidateSignersCtx() algorithm checks the condition t > n. However, as stated in the description, the valid range is 1 <= t <= n. Therefore, it would be clearer to replace the condition “Fail if t > n” with “Fail if not 1 <= t <= n”.

ValidateSignersCtx() checks the sanity of u signing participants (represented by ids and pubshares). So it only made sense to fail for all faults, like ids being out of range or containing duplicates. So, it's better to have it in the higher level?

I added has_duplicates again inside DeriveInterpolatingValue() because it has many other callers, just to be safe. Now that I look at it closely, all these calls get the ids & pubshares list after running ValidateSignersCtx() (through GetSessionValues), so we could technically remove the has_duplicates check from DeriveInterpolatingValue().

Therefore, it would be clearer to replace the condition “Fail if t > n” with “Fail if not 1 <= t <= n”.

I agree.

all these calls get the ids & pubshares list after running ValidateSignersCtx() (through GetSessionValues), so we could technically remove the has_duplicates check from DeriveInterpolatingValue().

Yes, I agree with you, since we call GetSessionValues() at the beginning of both functions that independently invoke DeriveInterpolatingValue(). Thus, we can remove the check from the DeriveInterpolatingValue() and keep it only in ValidateSignersCtx().

Alright. I'll update this.

I'd like to keep this redundant check in DeriveInterpolatingValue() for now. Just looked at RFC 9591 which defines their derive_interpolating_value function as:

def derive_interpolating_value(L, x_i):
if x_i not in L:
raise "invalid parameters"
for x_j in L:
if count(x_j, L) > 1:
raise "invalid parameters"
...
...

We can remove this has_duplicates check later, if we really need to.

While reading the implementation, I noticed that it includes a check ensuring that k_1 != k_2. At first glance, omitting this check does not appear to introduce any vulnerabilities, and we have verified this. However, I would appreciate hearing your opinion on this point.

Which function are you referring to? I don't see a k_1 != k_2 check in the nonce_gen_internal or deterministic_sign functions.

This is an interesting question though. I never considered adding this check, primarily because BIP327's reference implementation doesn't have it either.

Which function are you referring to? I don't see a k_1 != k_2 check in the nonce_gen_internal or deterministic_sign functions.

I apologize for not mentioning which implementation I was referring to. I meant the secp256k1-zkp FROST module, where the secp256k1_frost_nonce_gen() function performs this check. However, I think this verification is redundant, as I have not found any paper or specification that requires it. At first glance, it may seem that manipulation with Wagner attacks could apply, but I do not see any concrete attack vectors.

Oh, I was not aware of this. Thank you! I checked the Olaf paper as well, and it didn't have this requirement. I haven't thought about this from security proof perspective. Will keep this open till then :)

"Should we add a paragraph about OP_CHECKSIGADD like BIP327 does?"

Sounds reasonable to add that. If I'm not missing anything, the two paragraphs in BIP327 would fully apply to FROST as well (after minor adaptions, s/MuSig2/FROST/ and s/n-of-n/t-of-n/).

nit: should "MuSig2" replaced with "FROST" here?

nit: the constant is also available as Scalar.SIZE in secp256k1lab

(since infinity is a property rather than a method within GE)

pedantic nit: could swap these two lines, so the order matches the one of unpacking tweak_ctx (and the order of TweakCtxInit return values) below

nit: could use scalar_to_bytes here instead

pedantic nit, for consistency with other functions:

I believe this test should be moved into sign_error_test_cases since this would cause an error when deriving the threshold public key from the pubshares?

https://github.com/bitcoin/bips/blob/ec46a20323840b1a6aba83bc2d18b34dd0811245/bip-frost-signing.md#signers-context

Fail if DeriveThreshPubkey(id1..u, pubshare1..u) ≠ thresh_pk